Skip to content

The code reviewer reads the diff, not the plan#87

Merged
czpython merged 1 commit into
mainfrom
commonzenpython/reviewer-reads-the-diff
Jul 25, 2026
Merged

The code reviewer reads the diff, not the plan#87
czpython merged 1 commit into
mainfrom
commonzenpython/reviewer-reads-the-diff

Conversation

@czpython

Copy link
Copy Markdown
Owner

On PR #86 the reviewer approved with "cleanly delivers both payoff items … following the repo's existing idiom … and the plan's stated approach exactly" and filed zero findings, while the diff carried a redundant double-404, a flag standing in for an unnamed error path, and a test writing straight into DBOS's status table. Its own WHAT TO LOOK FOR list already names dead branches, foot-guns, reuse, and test shape. It never got that far: every clause of its verdict grades plan conformance, which is the evaluator's job and which this prompt explicitly forbids it from redoing.

It does that because of what it is handed. All seven roles include one _header.md, which renders the plan markdown, every acceptance criterion, the ruled-out list, prior evaluations, human feedback, and a MANDATORY FIRST ACTION to fetch the ticket as binding truth. The diff appears only as two SHAs.

The split

_contract.md now carries the ticket-fetch order, the plan, the acceptance criteria, the ruled-out approaches, prior evaluations, and human feedback. The six contract-bearing roles include it immediately after _header.md, so their prompts keep every section in the same order.

review_code.md includes _header.md alone — workflow context, workspace paths, verification, base_sha/head_sha — and gains one mandatory first action: git diff base..head, then read every changed file end to end, before any finding. It fetches the ticket when it needs scope to judge a finding and to file the follow-up sub-issue, rather than opening with it as a rubric.

Review prompt changes

  • Added to WHAT TO LOOK FOR: edits outside the ticket scope, named by file path — the class only a reader of the diff can catch. PR ENG-736 - Delete model_prefixes: route models off the harness's own fetched-or-shipped list #82's unrelated startswithtext[:3] rewrite in subprocess.py is the live example; it is in main.
  • Added out-of-scope edits at high severity.
  • Removed from WHAT NOT TO FLAG: "Style nits that don't change behavior or maintainability (whitespace, alphabetical import order, single vs double quote unless the repo enforces one)" — it read as licence to ignore house style.

Lint

ignore = ["SIM108"]. It rewrites if/else into a ternary; formatter-wrapped ternaries are banned here, so the lint gate was arguing against the house form.

Tests

test_contract_context_is_omitted_only_from_code_review renders all seven roles against a fully-populated journal and asserts the six carry every contract marker in order while the reviewer carries none of them and does carry the diff instruction.

test_build_prompt_orders_the_ticket_fetch now runs over _CONTRACT_TEMPLATES — the reviewer is deliberately outside that set.

Note

The six roles are not byte-identical to main: the ticket-fetch paragraph moved from the Workflow context bullets down into _contract.md, since the ticket is the contract. Section order and content are otherwise unchanged.

uv run ruff check backend, uv run ruff format --check backend, and backend/tests/test_build_prompts.py (23 passed) are green locally; the rest gates in CI.

All seven build roles shared one header, so the reviewer arrived holding the
plan, the acceptance criteria, every prior evaluation, and an order to treat
the ticket as binding truth — and graded plan conformance, which is the
evaluator's job it is told not to redo. The contract now lives in its own
partial that the six contract-bearing roles include; the reviewer gets the
workflow context and the SHAs, and is ordered to diff them and read every
changed file end to end before it writes a finding.

Its severity list gains out-of-scope edits at high — the one class only a
reader of the diff can see — and loses the bullet that waved off style as long
as the repo did not enforce it, which read as licence to ignore house style.

SIM108 is ignored: it rewrites if/else into a ternary, and plain if/else is
the house form.
@czpython
czpython merged commit af7980f into main Jul 25, 2026
1 check passed
@czpython
czpython deleted the commonzenpython/reviewer-reads-the-diff branch July 25, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant